Updated Signal Handler #287
Conversation
NoMoreFood
commented
Mar 16, 2018
•
NoMoreFood
commented
- Updated wait_for_multiple_objects_enhanced() to handle a no-event request while alertable.
- Removed unnecessary ZeroMemory() call in wait_for_multiple_objects_enhanced().
- Simplified wait_for_any_event() to by taking advantage of no-event alterable request in wait_for_multiple_objects_enhanced().
- Updated wait_for_any_event() to use MAX_CHILDREN limit instead of MAXIMUM_WAIT_OBJECTS limit.
- Updated wait_for_multiple_objects_enhanced() to handle a no-event request while alterable. - Simplified wait_for_any_event() to by taking advantage of no-event alterable request in wait_for_multiple_objects_enhanced(). - Updated wait_for_any_event() to use MAX_CHILDREN limit instead of MAXIMUM_WAIT_OBJECTS limit. - Removed unnecessary ZeroMemory() call.
contrib/win32/win32compat/signal.c
Outdated
| @@ -260,59 +260,43 @@ sw_process_pending_signals() | |||
| int | |||
| wait_for_any_event(HANDLE* events, int num_events, DWORD milli_seconds) | |||
| { | |||
| HANDLE all_events[MAXIMUM_WAIT_OBJECTS]; | |||
| DWORD num_all_events; | |||
| HANDLE all_events[MAX_CHILDREN]; | |||
There was a problem hiding this comment.
Does it make sense to make it static? We'll end up in this call quite often and don't want to stack allocate each time.
Also need to update the macro. See below comment.
There was a problem hiding this comment.
Your call. Performance of stack memory operations are typically constant (i.e. independent of size) since you just end up moving a single pointer higher/lower in the stack as you "allocate" and "free"; how much you move the pointer doesn't really matter unless you're worried about recursion / blowing your stack which is not a concern in this case. I think the risk of marking it static and making function no longer thread-safe and would outweigh any potential gain.
contrib/win32/win32compat/signal.c
Outdated
| if ((r = memcpy_s(all_events, MAXIMUM_WAIT_OBJECTS * sizeof(HANDLE), children.handles, live_children * sizeof(HANDLE)) != 0) || | ||
| ( r = memcpy_s(all_events + live_children, (MAXIMUM_WAIT_OBJECTS - live_children) * sizeof(HANDLE), events, num_events * sizeof(HANDLE)) != 0)) { | ||
| if ((r = memcpy_s(all_events, MAX_CHILDREN * sizeof(HANDLE), children.handles, live_children * sizeof(HANDLE)) != 0) || | ||
| ( r = memcpy_s(all_events + live_children, (MAX_CHILDREN - live_children) * sizeof(HANDLE), events, num_events * sizeof(HANDLE)) != 0)) { | ||
| debug3("memcpy_s failed with error: %d.", r); | ||
| return -1; | ||
| } | ||
|
|
||
| debug5("wait() on %d events and %d children", num_events, live_children); | ||
| /* TODO - implement signal catching and handling */ |
There was a problem hiding this comment.
you may remove this obsolete comment.
contrib/win32/win32compat/signal.c
Outdated
| num_all_events = num_events + live_children; | ||
|
|
||
| if (num_all_events > MAXIMUM_WAIT_OBJECTS) { | ||
| if (num_all_events > MAX_CHILDREN) { |
There was a problem hiding this comment.
We'll need a different MACRO named MAXIMUM_WAIT_OBJECTS_ENHANCED that can accommodate typical MAX_CHILDREN and total count of other events that can be passed to the wait* call.
You'll also need to update SELECT_EVENT_LIMIT (currently at 32) - see w32_select() implementation.
Typical scenario that would hit the SELECT_EVENT_LIMIT is a client forwarding a whole bunch of local ports (in which case it has to listen on each such port - each of which ends up as an additional event in the wait() call via select())
Recommend the following:
MAX_CHILDREN 512
SELECT_EVENT_LIMIT 512
MAXIMUM_WAIT_OBJECTS_ENHANCED 1024
- Created distinct definition MAXIMUM_WAIT_OBJECTS_ENHANCED and modified functions to use it. - Upped w32_select() event limit.
|
@manojampalam Any idea whether it's something wrong with my submission that breaking appveyor? A clean checkout and build from my branch seems to work fine. |
contrib/win32/win32compat/signal.c
Outdated
| errno = ENOTSUP; | ||
| return -1; | ||
| } | ||
|
|
||
| if ((r = memcpy_s(all_events, MAXIMUM_WAIT_OBJECTS * sizeof(HANDLE), children.handles, live_children * sizeof(HANDLE)) != 0) || | ||
| ( r = memcpy_s(all_events + live_children, (MAXIMUM_WAIT_OBJECTS - live_children) * sizeof(HANDLE), events, num_events * sizeof(HANDLE)) != 0)) { | ||
| if ((r = memcpy_s(all_events, MAX_CHILDREN * sizeof(HANDLE), children.handles, live_children * sizeof(HANDLE)) != 0) || |
There was a problem hiding this comment.
It should be MAXIMUM_WAIT_OBJECTS_ENHANCED on this line and line below.
|
@bingbing8 can you please point @NoMoreFood to instructions on how to debug in AppVeyor environment? |
- Modified wait_for_multiple_objects_enhanced() to allow for 0 millisecond wait. - Corrected buffer sizes in event array memory copies.
|
@manojampalam Comments address. Turned AppVerifier on and resolved the problem. I was previously unaware of the tool so thank you for introducing me to it. |
